added the TileApi to control real colorful lifx tiles #15

mabels · 2019-09-13T21:04:00Z

I need to cleanup this PR some sanity checks are missing and
i was to lazy to build the tests.
I promise that I complete this missing stuff in the next weeks.
This PR is just an reminder to me that i had to completed this!

ristomatti · 2019-09-14T22:57:17Z

@mabels Based on a quick skim, your PR looks very promising! When finalising the work, please remember to:

update README.md
follow .eslintrc

Also note that you can use arrow functions even though it's not used much if at all on the existing codebase (the ES2015 rewrite was done quite literally overnight). At least I would prefer arrow functions especially when passing an inline function as a parameter. I can't speak for @Sawtaytoes though who set up this fork though.

mabels · 2019-09-23T09:43:02Z

Hi,

just completed this including tests. There had been 2 other pull request which should applied first.

#16
#17

Thx in advance

ristomatti · 2019-09-24T00:11:51Z

Massive undertaking. I'll try to review and test this as soon as possible. Sorry if it might take a couple of days! I do have a Tile set to test though.

ristomatti · 2019-10-04T16:18:30Z

@mabels I'm sorry I haven't still had time to review and test this. Just letting you know I haven't forgot this! I just noticed a PR submitted to node-lifx-lan a couple of days ago and it'd be cool to have this merged before that. 😂 The problem is though that I'm quite busy today and away from home the rest of the weekend. I'll try to find time today but it doesn't look too good.

ristomatti · 2019-10-04T16:35:28Z

@mabels BTW just noticed there seems to be a conflict due to merging your previous PR #17.

ristomatti · 2019-10-05T13:39:12Z

My plans for today/tomorrow got cancelled. I'll at least start reviewing/testing now.

ristomatti · 2019-10-05T14:08:12Z

@mabels All you need to do to fix the conflict is to drop the already merged commits 3de27da and
1cca93a and then rebase to upstream master.

ristomatti · 2019-10-05T14:12:58Z

It's actually not a good idea to review the code at it's current state as it contains the already merged changes. @mabels could you update your branch and force push the changes? I'll still test now if I get it to work with my Tiles.

ristomatti · 2019-10-05T15:16:05Z

Sorry a somewhat off-topic note:

Also note that you can use arrow functions even though it's not used much if at all on the existing codebase (the ES2015 rewrite was done quite literally overnight). At least I would prefer arrow functions especially when passing an inline function as a parameter. I can't speak for @Sawtaytoes though who set up this fork though.

Quoting myself - now when I started to review the changes @mabels has done, I realize there's still two open PR's from me that's aren't included in this repo. When commenting about the arrow functions, I had an odd feeling like I would have done "something" to that already: MariusRumpf/node-lifx#62. This is the rewrite I was talking about.

@Sawtaytoes It looks like I forgot PR 62 completely. Would it make sense for me to try to rebase it and do a PR? Or do you prefer working without the use of classes. I've personally been leaning towards more functinal programming approach (mainly at work) but I think using classes would make sense given how the library is written. There'd be a lot less clutter.

src/lifx/light.js

ristomatti

@mabels Added a couple of comments but I'll check the other changes after you've rebased the branch.

src/lifx/light.js

src/lifx/packets/stateDeviceChain.js

src/lifx/packets/stateTileState64.js

src/lifx/utils.js

Sawtaytoes · 2019-10-06T03:24:13Z

Sorry a somewhat off-topic note:

Also note that you can use arrow functions even though it's not used much if at all on the existing codebase (the ES2015 rewrite was done quite literally overnight). At least I would prefer arrow functions especially when passing an inline function as a parameter. I can't speak for @Sawtaytoes though who set up this fork though.

Quoting myself - now when I started to review the changes @mabels has done, I realize there's still two open PR's from me that's aren't included in this repo. When commenting about the arrow functions, I had an odd feeling like I would have done "something" to that already: MariusRumpf/node-lifx#62. This is the rewrite I was talking about.

@Sawtaytoes It looks like I forgot PR 62 completely. Would it make sense for me to try to rebase it and do a PR? Or do you prefer working without the use of classes. I've personally been leaning towards more functinal programming approach (mainly at work) but I think using classes would make sense given how the library is written. There'd be a lot less clutter.

Whatever your preference is fine. I think bringing in those old PRs from your other repo would be fantastic!

I also like arrow functions. As long as our Node.js compatibility is fine, then we're good to go; otherwise, we should just bring up the min Node.js version and everything will be easier going forward.

mabels · 2019-10-07T06:32:33Z

Node and Arrowfunctions:

for i in boron carbon dubnium  ST 1   master
do
nvm run lts/$i -e '(() => console.log(process.versions.node))()'
done
Running node v6.17.1 (npm v3.10.10)
Running node v8.16.1 (npm v6.4.1)
Running node v10.16.3 (npm v6.11.3)

we should never go to 4.x so on my expirence we are fine.
We could also rewrite arrow functions with babel

mabels · 2019-10-07T07:47:27Z

Sorry a somewhat off-topic note:

Also note that you can use arrow functions even though it's not used much if at all on the existing codebase (the ES2015 rewrite was done quite literally overnight). At least I would prefer arrow functions especially when passing an inline function as a parameter. I can't speak for @Sawtaytoes though who set up this fork though.

Quoting myself - now when I started to review the changes @mabels has done, I realize there's still two open PR's from me that's aren't included in this repo. When commenting about the arrow functions, I had an odd feeling like I would have done "something" to that already: MariusRumpf/node-lifx#62. This is the rewrite I was talking about.
@Sawtaytoes It looks like I forgot PR 62 completely. Would it make sense for me to try to rebase it and do a PR? Or do you prefer working without the use of classes. I've personally been leaning towards more functinal programming approach (mainly at work) but I think using classes would make sense given how the library is written. There'd be a lot less clutter.

Whatever your preference is fine. I think bringing in those old PRs from your other repo would be fantastic!

I also like arrow functions. As long as our Node.js compatibility is fine, then we're good to go; otherwise, we should just bring up the min Node.js version and everything will be easier going forward.

we should integrate "Arrow functions completly" after this PR is on master.
I offer my help.

ristomatti

Added few comments. My JS limit for today has maxed out now though. I'll see if I could get this running tomorrow or Sunday.

ristomatti · 2019-10-11T15:55:37Z

example/tile.js

+  console.log('New light found.');
+  console.log('ID: ' + light.id);
+
+  light.getDeviceChain(function(err, chain) {


This would now attempt to call setTileState64 on any light. Add a const TILE_LABEL = 'MyTile'; and trigger only on a light matching the label.

This does not work as light label has not resolved at this point. It was partly my bad as I didn't remember this while reviewing this the first time. It would improve the example a lot if a label could be used but for this purpose it would be enough to just rename TILE_LABEL TO TILE_LIGHT_ID for example.

example/tile.js

src/lifx/light.js

Sawtaytoes · 2019-12-07T13:55:14Z

@mabels @ristomatti Just your friendly neighborhood ping :).

ristomatti · 2019-12-08T14:52:21Z

@mabels @ristomatti Just your friendly neighborhood ping :).

I'm still waiting for @mabels. I guess eventually I could try sorting the issues myself but haven't had time to think about this too much unfortunately.

On a related note, I ran across this stuff that's based on the "competing" node-lifx-lan lib that looked pretty cool. Haven't tested it though:

object and callback. removed validation clutter from set/getTileState64 added some comments in the example

ristomatti · 2019-12-10T13:27:52Z

Thanks @mabels. I will check and test this later today (= within the next 12 hours). If I won't, you're welcome to merge this @Sawtaytoes.

Sawtaytoes · 2019-12-11T03:18:59Z

@ristomatti Merging in 1 hour.

ristomatti · 2019-12-12T10:08:21Z

@Sawtaytoes Thanks for the heads-up (although I saw it only just now). Sadly I was blocked by a force majeure.

Would it be possible for me to email or PM you on this? You can find my email from my commit messages. Your location indicates you're 8 hours behind but I'm a night owl so there should be plenty of hours reach me.

ristomatti · 2019-12-14T14:30:09Z

Impossible things can happen - testing now.

ristomatti · 2019-12-14T15:16:50Z

@mabels Did you test this after your changes? I can't get this to work. There's multiple issues. When running example/tile.js I got this:

$ node tile.js 
Started LIFX listening on 0.0.0.0:49419

New light found.
ID: d073d5394495
/home/ristomatti/Projects/lifx-lan-client/lib/lifx/light.js:657
  validate.isUint8(tileIndex, 'setTileState64', 'tileIndex');
           ^

TypeError: validate.isUint8 is not a function
    at Light.setTileState64 (/home/ristomatti/Projects/lifx-lan-client/lib/lifx/light.js:657:12)
    at setBits (/home/ristomatti/Projects/lifx-lan-client/example/tile.js:41:9)
    at /home/ristomatti/Projects/lifx-lan-client/example/tile.js:61:5
    at Client.<anonymous> (/home/ristomatti/Projects/lifx-lan-client/lib/lifx/light.js:534:12)

After replacing renaming 3 calls to validate.isUint8 with validate.isUInt8 it crashed like so:

$ node tile.js 
Started LIFX listening on 0.0.0.0:43892

New light found.
ID: d073d5394495
/home/ristomatti/Projects/lifx-lan-client/lib/lifx/utils.js:143
    throw new TypeError('LIFX util buildColorsHsbk expects colors to be an array');
    ^

TypeError: LIFX util buildColorsHsbk expects colors to be an array
    at Object.utils.buildColorsHsbk (/home/ristomatti/Projects/lifx-lan-client/lib/lifx/utils.js:143:11)
    at Light.setTileState64 (/home/ristomatti/Projects/lifx-lan-client/lib/lifx/light.js:671:27)
    at setBits (/home/ristomatti/Projects/lifx-lan-client/example/tile.js:41:9)
    at /home/ristomatti/Projects/lifx-lan-client/example/tile.js:61:5
    at Client.<anonymous> (/home/ristomatti/Projects/lifx-lan-client/lib/lifx/light.js:534:12)

ristomatti · 2019-12-14T15:25:05Z

@Sawtaytoes Sorry but this is not ready yet. I don't think we should rush this in as there might be other issues also. For instance README.md would need to be updated.

ristomatti · 2019-12-14T15:26:59Z

src/lifx/light.js

+ * @param {Number} tileIndex	unsigned 8-bit integer
+ * @param {Number} userX	32-bit float
+ * @param {Number} userY	32-bit float
+ * @param {Number} reserved	16-bit integer


What is this reserved value and how would a user of this library know what to put there? This applies to all functions.

src/lifx/light.js

mabels · 2019-12-14T18:21:48Z

with the unused options that is my fault. I will fix it and build a test that it not happend again meno

…

On Sat, 14 Dec 2019, 16:33 Ristomatti Airo, ***@***.***> wrote: ***@***.**** requested changes on this pull request. ------------------------------ In src/lifx/light.js <#15 (comment)> : > + const sqnNumber = this.client.send(packetObj); + this.client.addMessageHandler('stateDeviceChain', function(err, msg) { + if (err) { + return callback(err, null); + } + return callback(null, msg); + }, sqnNumber); +}; + +/** + * Sets Tile Position + * @example light.setUserPosition(0, 12, 13, 0, () => {}) + * @param {Number} tileIndex unsigned 8-bit integer + * @param {Number} userX 32-bit float + * @param {Number} userY 32-bit float + * @param {Number} reserved 16-bit integer What is this reserved value and how would a user of this library know what to put there? This applies to all functions. ------------------------------ In src/lifx/light.js <#15 (comment)> : > + * each tile. + * @param {Number} tileIndex unsigned 8-bit integer + * @param {Object} options - options passed to + * @param {Number} options.length unsigned 8-bit integer (default 64) + * @param {Number} options.x unsigned 8-bit integer (default 0) + * @param {Number} options.y unsigned 8-bit integer (default 0) + * @param {Number} options.width unsigned 8-bit integer (default 8) + * @param {Number} options.reserved unsigned 8-bit integer + * @param {Function} callback a function to accept the data + */ +Light.prototype.getTileState64 = function(tileIndex, options, callback) { + validate.isUint8(tileIndex, 'getTileState64', 'tileIndex'); + if (typeof options === 'function') { + callback = options; + options = {}; + } What's the purpose of the above block? ------------------------------ In src/lifx/light.js <#15 (comment)> : > + * @param {Number} colors[64] 64 HSBK values + * @param {Object} options - options passed to + * @param {Number} options.length unsigned 8-bit integer (default 0) + * @param {Number} options.x unsigned 8-bit integer (default 8) + * @param {Number} options.y unsigned 8-bit integer (default 8) + * @param {Number} options.width unsigned 8-bit integer (default 8) + * @param {Number} options.duration unsigned 32-bit integer (default 0) + * @param {Number} options.reserved unsigned 8-bit integer + * @param {Function} [callback] called when light did receive message + */ +Light.prototype.setTileState64 = function(tileIndex, colors, options, callback) { + validate.isUint8(tileIndex, 'setTileState64', 'tileIndex'); + if (typeof options === 'function') { + callback = options; + options = {}; + } What's the purpose of the above block? Also note that options is assigned to {} but options is not used after this. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15?email_source=notifications&email_token=AAAEWJWF5L7P4JR5RRZJP3TQYT4DJA5CNFSM4IWUA6Y2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCPGUDFY#pullrequestreview-332218775>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAEWJRMQTAPM6KLROLHV2DQYT4DJANCNFSM4IWUA6YQ> .

mabels · 2019-12-14T21:52:36Z

@mabels Did you test this after your changes? I can't get this to work. There's multiple issues. When running example/tile.js I got this:

$ node tile.js 
Started LIFX listening on 0.0.0.0:49419

New light found.
ID: d073d5394495
/home/ristomatti/Projects/lifx-lan-client/lib/lifx/light.js:657
  validate.isUint8(tileIndex, 'setTileState64', 'tileIndex');
           ^

TypeError: validate.isUint8 is not a function
    at Light.setTileState64 (/home/ristomatti/Projects/lifx-lan-client/lib/lifx/light.js:657:12)
    at setBits (/home/ristomatti/Projects/lifx-lan-client/example/tile.js:41:9)
    at /home/ristomatti/Projects/lifx-lan-client/example/tile.js:61:5
    at Client.<anonymous> (/home/ristomatti/Projects/lifx-lan-client/lib/lifx/light.js:534:12)

After replacing renaming 3 calls to validate.isUint8 with validate.isUInt8 it crashed like so:

$ node tile.js 
Started LIFX listening on 0.0.0.0:43892

New light found.
ID: d073d5394495
/home/ristomatti/Projects/lifx-lan-client/lib/lifx/utils.js:143
    throw new TypeError('LIFX util buildColorsHsbk expects colors to be an array');
    ^

TypeError: LIFX util buildColorsHsbk expects colors to be an array
    at Object.utils.buildColorsHsbk (/home/ristomatti/Projects/lifx-lan-client/lib/lifx/utils.js:143:11)
    at Light.setTileState64 (/home/ristomatti/Projects/lifx-lan-client/lib/lifx/light.js:671:27)
    at setBits (/home/ristomatti/Projects/lifx-lan-client/example/tile.js:41:9)
    at /home/ristomatti/Projects/lifx-lan-client/example/tile.js:61:5
    at Client.<anonymous> (/home/ristomatti/Projects/lifx-lan-client/lib/lifx/light.js:534:12)

I could not test this change off course i had no lifx on hand.
I will try this if i come back home in the next week.

mabels · 2019-12-14T21:56:09Z

@mabels Did you test this after your changes? I can't get this to work. There's multiple issues. When running example/tile.js I got this:

$ node tile.js 
Started LIFX listening on 0.0.0.0:49419

New light found.
ID: d073d5394495
/home/ristomatti/Projects/lifx-lan-client/lib/lifx/light.js:657
  validate.isUint8(tileIndex, 'setTileState64', 'tileIndex');
           ^

TypeError: validate.isUint8 is not a function
    at Light.setTileState64 (/home/ristomatti/Projects/lifx-lan-client/lib/lifx/light.js:657:12)
    at setBits (/home/ristomatti/Projects/lifx-lan-client/example/tile.js:41:9)
    at /home/ristomatti/Projects/lifx-lan-client/example/tile.js:61:5
    at Client.<anonymous> (/home/ristomatti/Projects/lifx-lan-client/lib/lifx/light.js:534:12)

After replacing renaming 3 calls to validate.isUint8 with validate.isUInt8 it crashed like so:

$ node tile.js 
Started LIFX listening on 0.0.0.0:43892

New light found.
ID: d073d5394495
/home/ristomatti/Projects/lifx-lan-client/lib/lifx/utils.js:143
    throw new TypeError('LIFX util buildColorsHsbk expects colors to be an array');
    ^

TypeError: LIFX util buildColorsHsbk expects colors to be an array
    at Object.utils.buildColorsHsbk (/home/ristomatti/Projects/lifx-lan-client/lib/lifx/utils.js:143:11)
    at Light.setTileState64 (/home/ristomatti/Projects/lifx-lan-client/lib/lifx/light.js:671:27)
    at setBits (/home/ristomatti/Projects/lifx-lan-client/example/tile.js:41:9)
    at /home/ristomatti/Projects/lifx-lan-client/example/tile.js:61:5
    at Client.<anonymous> (/home/ristomatti/Projects/lifx-lan-client/lib/lifx/light.js:534:12)

I could not test this change off course i had no lifx on hand.
I will try this if i come back home in the next week.

mabels · 2019-12-14T21:56:51Z

I need to build a test for the high order function. I'm sorry for this chain of failures.

mabels force-pushed the master branch 2 times, most recently from acad593 to 88691bb Compare September 23, 2019 09:40

mabels marked this pull request as ready for review September 23, 2019 09:41

ristomatti self-requested a review October 4, 2019 16:33

ristomatti reviewed Oct 5, 2019

View reviewed changes

src/lifx/light.js Outdated Show resolved Hide resolved

ristomatti requested changes Oct 5, 2019

View reviewed changes

ristomatti mentioned this pull request Oct 5, 2019

Refactor function prototypes to classes, use arrow function instead of bind #19

Open

mabels force-pushed the master branch from 88691bb to b650653 Compare October 7, 2019 07:41

added the TileApi to control real colorful lifx tiles

f4b20a9

mabels force-pushed the master branch from b650653 to f4b20a9 Compare October 7, 2019 07:42

mabels requested a review from ristomatti October 9, 2019 10:26

ristomatti reviewed Oct 11, 2019

View reviewed changes

changed api of set/getTileState64 to have an options

f826c27

object and callback. removed validation clutter from set/getTileState64 added some comments in the example

Sawtaytoes approved these changes Dec 10, 2019

View reviewed changes

ristomatti requested changes Dec 14, 2019

View reviewed changes

fix in setTileState64 the missing option passing

2b21d02

fix: the typo of Uint8 to UInt8

9052bd2

mabels closed this Dec 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added the TileApi to control real colorful lifx tiles #15

added the TileApi to control real colorful lifx tiles #15

mabels commented Sep 13, 2019

ristomatti commented Sep 14, 2019

mabels commented Sep 23, 2019

ristomatti commented Sep 24, 2019

ristomatti commented Oct 4, 2019

ristomatti commented Oct 4, 2019

ristomatti commented Oct 5, 2019

ristomatti commented Oct 5, 2019

ristomatti commented Oct 5, 2019

ristomatti commented Oct 5, 2019

ristomatti left a comment

Sawtaytoes commented Oct 6, 2019 •

edited

Loading

mabels commented Oct 7, 2019

mabels commented Oct 7, 2019

ristomatti left a comment

ristomatti Oct 11, 2019

mabels Dec 10, 2019

ristomatti Dec 14, 2019

Sawtaytoes commented Dec 7, 2019

ristomatti commented Dec 8, 2019

ristomatti commented Dec 10, 2019

Sawtaytoes commented Dec 11, 2019

ristomatti commented Dec 12, 2019 •

edited

Loading

ristomatti commented Dec 14, 2019

ristomatti commented Dec 14, 2019

ristomatti commented Dec 14, 2019

ristomatti Dec 14, 2019

mabels commented Dec 14, 2019 via email

mabels commented Dec 14, 2019

mabels commented Dec 14, 2019

mabels commented Dec 14, 2019

added the TileApi to control real colorful lifx tiles #15

added the TileApi to control real colorful lifx tiles #15

Conversation

mabels commented Sep 13, 2019

ristomatti commented Sep 14, 2019

mabels commented Sep 23, 2019

ristomatti commented Sep 24, 2019

ristomatti commented Oct 4, 2019

ristomatti commented Oct 4, 2019

ristomatti commented Oct 5, 2019

ristomatti commented Oct 5, 2019

ristomatti commented Oct 5, 2019

ristomatti commented Oct 5, 2019

ristomatti left a comment

Choose a reason for hiding this comment

Sawtaytoes commented Oct 6, 2019 • edited Loading

mabels commented Oct 7, 2019

mabels commented Oct 7, 2019

ristomatti left a comment

Choose a reason for hiding this comment

ristomatti Oct 11, 2019

Choose a reason for hiding this comment

mabels Dec 10, 2019

Choose a reason for hiding this comment

ristomatti Dec 14, 2019

Choose a reason for hiding this comment

Sawtaytoes commented Dec 7, 2019

ristomatti commented Dec 8, 2019

ristomatti commented Dec 10, 2019

Sawtaytoes commented Dec 11, 2019

ristomatti commented Dec 12, 2019 • edited Loading

ristomatti commented Dec 14, 2019

ristomatti commented Dec 14, 2019

ristomatti commented Dec 14, 2019

ristomatti Dec 14, 2019

Choose a reason for hiding this comment

mabels commented Dec 14, 2019 via email

mabels commented Dec 14, 2019

mabels commented Dec 14, 2019

mabels commented Dec 14, 2019

Sawtaytoes commented Oct 6, 2019 •

edited

Loading

ristomatti commented Dec 12, 2019 •

edited

Loading